🤖 Kimi Code Review

The PR looks solid. This correctly implements protection against blob-count griefing attacks in the mempool (where a user replaces a high-blob transaction with a low-blob one to cheaply cycle gossip).

Feedback:

1. Test completeness in crates/blockchain/mempool.rs
The tests verify error returns but don't assert that the mempool state remains unchanged after a failed replacement. While find_tx_to_replace returns an error (preventing replacement), consider adding an assertion that the old transaction hash is still present in the pool after the rejection:

// After the match block in rejects_replacement_with_fewer_blobs_even_with_doubled_fees
assert!(mempool.contains(&old.hash())); // Ensure old tx wasn't evicted

2. Code clarity in blob comparison (line ~475)
The current logic is correct, but consider extracting the condition to make the guard more explicit:

let shrinking_blobs = new_count < old_count;
if shrinking_blobs {
    return Err(...);
}

This reads slightly better than the inline comparison, though this is subjective.

3. TODO comment accuracy (line ~468)
The TODO references PR #6601 for type-discriminant checking. Ensure that when #6601 lands, this code is updated to remove the redundant match arms. The current implementation is safe as a stopgap.

Security verification:

• Correctly prevents the DoS vector: A 6-blob → 1-blob replacement is rejected even with 100% fee bumps (lines 607-630)
• Allows legitimate increases: 1-blob → 6-blob with proper fees is accepted (lines 665-683)
• Maintains consistency: Same-count replacements work as expected (lines 637-659)

Nitpick:
In make_blob_tx, using H256::from_low_u64_be(i as u64 + 1) for blob hashes is fine for tests, though these aren't valid KZG commitments. This is acceptable for unit tests since the mempool doesn't validate cryptographic correctness of blobs, only counts them.

Conclusion: LGTM. The logic is correct, well-documented, and the tests cover the three critical cases (fewer, equal, greater blob counts).

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

🤖 Claude Code Review

PR Review: feat(l1): reject blob-tx replacements that shrink the blob count

Overview

This PR closes a residual blob-spam vector left open after #6601. An attacker could satisfy an RBF fee bump while replacing a many-blob EIP-4844 tx with a fewer-blob one, cycling cheaply to amplify blob churn relative to fees paid. The fix adds a blob-count monotonicity invariant to find_tx_to_replace and is correctly described as purely additive.

Correctness

The core logic is correct. The guard at mempool.rs:473–484 fires before the fee-bump check, so a shrinking replacement is rejected even when fee thresholds are met. The comparison new_count < old_count is the right predicate (same count and growing are both allowed).

Cross-type RBF gap (design limitation — worth documenting explicitly). The if let pattern only matches when both transactions are EIP4844Transaction. If an attacker replaces a 6-blob EIP-4844 tx with an EIP-1559 (or legacy) tx, the pattern does not match and the blob count check is bypassed entirely. The eip4844_higher_fees gate below then defaults to true (see mempool.rs:505–507), so the non-blob replacement can succeed with just an EIP-1559 fee bump. This means the spam vector is not fully closed for cross-type replacements.

The TODO comment acknowledges this will be covered by the type-discriminant check in #6601 — that's a reasonable forward reference. But the current test suite has no case documenting this expected behavioral gap, which makes the limitation invisible to future readers. Consider adding:

#[test]
fn cross_type_replacement_eip4844_to_eip1559_bypasses_blob_check() {
    // Documents the current intentional gap:
    // replacing EIP-4844 with EIP-1559 bypasses the blob-count invariant
    // (will be closed by PR #6601's type-discriminant guard).
    ...
}

Existing pre-existing concern (out of scope, but worth noting)

The fee check at mempool.rs:501–507 uses strict > rather than a percentage bump:

new_blob_fee > old_blob_fee

This is a pre-existing issue unrelated to this PR, but the description says #6601 "introduces percentage fee bumps" — it would be worth verifying the integration once #6601 lands.

Tests

The three tests (rejects_replacement_with_fewer_blobs_even_with_doubled_fees, accepts_replacement_with_same_blob_count_and_doubled_fees, accepts_replacement_with_more_blobs_and_doubled_fees) are well-structured and cover all branches of the new guard. Specifically:

• The rejection test correctly verifies both old_count and new_count values in the error, not just the error variant.
• insert_tx panics on failure, which is appropriate for test setup.
• H256::from_low_u64_be(i+1) produces syntactically fake versioned hashes (real ones must start with 0x01), but that's acceptable for unit tests that don't exercise hash validation.

One minor improvement — the DOUBLED_FEE_MULTIPLIER constant is helpful but the 100% bump it represents is never connected to any fee-bump policy enforcement. A doc comment clarifying it's just "2× to satisfy the existing > check" would prevent confusion when #6601 introduces actual percentage thresholds.

Error Variant

ReplacementShrinksBlobs { old_count: usize, new_count: usize } is well-named and the #[error(...)] message is informative. usize is appropriate for counts derived from Vec::len().

Summary

The implementation is focused, minimal, and correctly closes the same-type blob-shrink vector. The main gap is cross-type RBF (EIP4844 → EIP1559), which is a known limitation acknowledged by the TODO, but worth making explicit with a test. No other correctness, security, or safety issues found.

Automated review by Claude (Anthropic) · sonnet · custom prompt

Greptile Summary

This PR closes a residual blob-spam vector in the mempool's Replace-By-Fee logic: a caller could previously satisfy the fee-bump threshold while replacing a 6-blob EIP-4844 transaction with a 1-blob one, amplifying blob churn at low cost. The fix adds a single guard in find_tx_to_replace that rejects any EIP-4844 replacement carrying fewer blob_versioned_hashes than the pooled transaction, firing before the per-fee-field bump check.

• New MempoolError::ReplacementShrinksBlobs { old_count, new_count } variant with a descriptive message is added to error.rs.
• Three unit tests cover the reject (shrink), accept (same count), and accept (grow) paths, seeded through Mempool::add_transaction to exercise the real pool state.

Confidence Score: 4/5

Safe to merge; the change is purely additive and tightens RBF admission for EIP-4844 transactions.

The blob-count guard fires correctly before the fee check, and the three tests exercise the reject/same/grow paths against real pool state. The only remaining gap — replacing an EIP-4844 pooled tx with a non-EIP-4844 tx bypasses this check entirely — is pre-existing, acknowledged with a TODO, and tracked in #6601/#6623. Nothing in this PR regresses existing behaviour.

No files require special attention, though mempool.rs would benefit from an additional test covering the cross-type replacement path (EIP-4844 in pool, non-EIP-4844 replacement) once #6601 lands.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[find_tx_to_replace called] --> B{contains_sender_nonce?}
    B -->|None| C[return Ok None]
    B -->|Some tx_in_pool| D{Both txs are EIP4844Transaction?}
    D -->|No| F[fee bump check]
    D -->|Yes| E{new blob count less than old blob count?}
    E -->|Yes| G[return Err ReplacementShrinksBlobs]
    E -->|No - same or more| F
    F -->|fails| H[return Err UnderpricedReplacement]
    F -->|passes| I[return Ok Some old_hash]

_{Reviews (1): Last reviewed commit: "feat(l1): reject blob-tx replacements th..." | Re-trigger Greptile}

Lines of code report

Total lines added: 154
Total lines removed: 0
Total lines changed: 154

+-------------------------------------+-------+------+
| File                                | Lines | Diff |
+-------------------------------------+-------+------+
| ethrex/crates/blockchain/error.rs   | 151   | +4   |
+-------------------------------------+-------+------+
| ethrex/crates/blockchain/mempool.rs | 603   | +150 |
+-------------------------------------+-------+------+

🤖 Codex Code Review

Findings

1. The new guard is still bypassable for cross-type replacements. In mempool.rs, ReplacementShrinksBlobs only triggers for EIP4844 -> EIP4844. But blockchain.rs still routes plain transactions through the same find_tx_to_replace(), and mempool.rs:501-506 treats new_tx.max_fee_per_blob_gas() == None as true. That means an attacker can still replace an in-pool blob tx with a higher-fee non-blob tx and shrink the sidecar from N blobs to 0, which leaves the intended DoS fix incomplete. I’d enforce tx_type() equality here now, or at least reject old_blob && !new_blob. The added tests in mempool.rs only cover 4844 -> 4844, so this bypass would not be caught.

No other correctness or security issues stood out in the diff.

I couldn’t run cargo test in this environment because cargo/rustup failed to create temp files under the read-only /home/runner/.rustup/tmp.

Automated review by OpenAI Codex · gpt-5.4 · custom prompt